Skip to content

Port TypeScript PR #60321: Error on too many parameters for iterator method #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Port TypeScript PR #60321: Error on too many parameters for iterator method #16

wants to merge 10 commits into from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Jun 6, 2025 •
edited
Loading

This PR ports TypeScript PR #60321 which adds validation to ensure iterator methods ([Symbol.iterator] and [Symbol.asyncIterator]) have zero required parameters.

Problem

Iterator methods are called without arguments by JavaScript language constructs like for..of loops, yield*, and spread syntax. When these methods have required parameters, runtime errors occur:

const iter = {
    *[Symbol.iterator](_: number) {  // ❌ Required parameter
        yield 0;
    }
};

for (const _ of iter);  // Runtime error: method expects 1 argument, got 0
yield* iter;            // Runtime error: method expects 1 argument, got 0

Solution

Modified getIterationTypesOfIterableSlow in internal/checker/checker.go to:

  1. Filter signatures to only include those with getMinArgumentCount(sig) == 0
  2. Report detailed errors when iterator methods have required parameters using checkTypeAssignableTo
  3. Provide clear diagnostics explaining the parameter count mismatch

Results

The implementation now correctly reports detailed compile-time errors:

error TS2322: Type '{ [Symbol.iterator](_: number): Generator<number, void, unknown>; }' is not assignable to type 'Iterable<T, TReturn, TNext>'.
  Types of property '[Symbol.iterator]' are incompatible.
    Type '(_: number) => Generator<number, void, unknown>' is not assignable to type '() => Iterator<T, TReturn, TNext>'.
      Target signature provides too few arguments. Expected 1 or more, but got 0.

Working correctly:

  • ✅ for (const _ of iter) - Reports TS2322 + TS2488 with parameter details
  • ✅ yield* iter - Reports TS2322 + TS2488 with parameter details
  • ✅ for await (const _ of iter) - Reports TS2504 for async iterators

This prevents runtime errors by catching iterator parameter mismatches at compile time, helping developers write more robust code.

Fixes #14.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Port #60321 Port TypeScript PR #60321: Error on too many parameters for iterator method Jun 6, 2025
@Copilot Copilot AI requested a review from andrewbranch June 6, 2025 16:34
Copilot finished work on behalf of andrewbranch June 6, 2025 16:34
andrewbranch
andrewbranch requested changes Jun 6, 2025
View reviewed changes
Copy link
Owner

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks close, but the error diffs show that the original output had one top-level error TS2504 (must have a Symbol.asyncInterator method...) with an attached related TS2322 (Type is not assignable to type...), whereas your change issues those two errors both as top-level. If you use c.checkTypeAssignableToEx instead of c.checkTypeAssignableTo, you can pass along the diagnosticOutput pointer. Maybe that will combine the two errors into one chain.

@Copilot Copilot SWE Agent
Copy link
Author

Copilot AI commented Jun 6, 2025

This looks close, but the error diffs show that the original output had one top-level error TS2504 (must have a Symbol.asyncInterator method...) with an attached related TS2322 (Type is not assignable...

Fixed in commit 079190c. Changed from checkTypeAssignableTo to checkTypeAssignableToEx with the diagnosticOutput parameter. The TS2322 errors are now properly attached as related information to the main TS2488/TS2504 errors instead of being separate top-level errors. Error count reduced from 4 to 2 as expected.

Copilot finished work on behalf of andrewbranch June 6, 2025 16:59
@andrewbranch
Copy link
Owner

@copilot the asyncIteratorExtraParameters test looks perfect (the error diff is deleted), but the iteratorExtraParameters test is missing errors on the array and parameter spread lines. Your change in checker.go looks like a complete and correct port to me, so something must be different about the code paths between the TypeScript repo and Go repo here. Can you investigate?

@Copilot Copilot SWE Agent
Copy link
Author

Copilot AI commented Jun 6, 2025

@copilot the asyncIteratorExtraParameters test looks perfect (the error diff is deleted), but the iteratorExtraParameters test is missing errors on the array and parameter spread lines. Your chang...

I've identified and partially fixed the issue. The missing errors for spread syntax were caused by a caching problem in getIterationTypesOfIterable.

Root cause: The cache key calculation in IterationUseCacheFlags didn't include the distinguishing flag bits (IterationUseYieldStarFlag, IterationUseSpreadFlag, etc.), causing different iteration uses to share the same cache entry and bypass validation.

Fix applied: Updated IterationUseCacheFlags to include the missing flag bits.

Current status:

  • ✅ for (const _ of iter) - working
  • ✅ yield* iter - working
  • ✅ [...iter] - now working (fixed by cache fix)
  • ❌ g(...iter) - still investigating different code path

The function call spread appears to take a different validation path than array literal spread. Working on identifying why the final case isn't being caught.

Copilot finished work on behalf of andrewbranch June 6, 2025 18:32
@andrewbranch
Copy link
Owner

@copilot can you run the tests and update the baselines?

@Copilot Copilot SWE Agent
Copy link
Author

Copilot AI commented Jun 6, 2025

@copilot can you run the tests and update the baselines?

Tests run and baselines updated in commit 7273d6d. The iteratorExtraParameters test now correctly reports 3 errors instead of 2, including the missing error for [...iter] spread syntax. The asyncIteratorExtraParameters test continues to work perfectly with 2 errors as expected.

Copilot finished work on behalf of andrewbranch June 6, 2025 20:45
@andrewbranch
Copy link
Owner

@copilot can you investigate the final missing error in iteratorExtraParameters, the one on spread args g(...iter)

@Copilot Copilot SWE Agent
Copy link
Author

Copilot AI commented Jun 6, 2025

@copilot can you investigate the final missing error in iteratorExtraParameters, the one on spread args g(...iter)

Fixed in commit 8e97b45. The missing error for g(...iter) was caused by an iterator type caching issue. When array spread [...iter] was processed first, it cached an empty iteration result which prevented subsequent function call spread g(...iter) from triggering validation.

Modified the cache logic to avoid caching empty results when error reporting is enabled, ensuring each call site gets proper validation. All 4 expected errors are now correctly reported.

Copilot finished work on behalf of andrewbranch June 6, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewbranch andrewbranch andrewbranch requested changes

Assignees

@andrewbranch andrewbranch

Copilot code review Copilot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Port #60321
2 participants
@Copilot @andrewbranch